Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Docker CLI fallback if Podman is not found #165

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Add Docker CLI fallback if Podman is not found #165

merged 1 commit into from
Mar 4, 2024
+34 −9

Conversation

dagood
Copy link
Contributor

@dagood dagood commented Feb 28, 2024

Add a simple fallback to the logic that detects podman to also try to detect docker.

Related:

@dagood dagood force-pushed the dev/dagood/docker-fallback branch from c089354 to f47b209 Compare February 28, 2024 00:40
@aufi aufi requested a review from eemcmullan February 28, 2024 12:57
@shawn-hurley shawn-hurley self-requested a review February 28, 2024 14:53
pranavgaikwad
pranavgaikwad requested changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@pranavgaikwad pranavgaikwad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if CONTAINER_RUNTIME would be a better name instead of PODMAN_BIN now that we know docker can be used as alternative? wdyt @dagood @eemcmullan @shawn-hurley

@shawn-hurley
Copy link
Contributor

I am ok with PODMAN_BIN. I would worry about container_runtime, because I think that refers to something other than what we are using.

CONTAINER_TOOL or something might work though?

@pranavgaikwad
Copy link
Contributor

You're right, that would be confusing...nvm, thanks!

@eemcmullan eemcmullan merged commit 7236f3f into konveyor:main Mar 4, 2024
2 checks passed
@dagood
Copy link
Contributor Author

dagood commented Mar 4, 2024

Thanks for the reviews and merge!

I do think that a neutral name could make it easier to document (and understand)--the name PODMAN_BIN makes a pretty strong first impression. 😄 I was considering e.g. loadDefaultContainerizationCLI instead of loadDefaultPodmanBin, but stuck with PodmanBin to match the env var for now. I didn't want to propose changing the env var name at the same time as this PR because picking a good name might need some discussion, the change might need some iteration to find usages (and maybe break some people downstream), and auto-fallback makes the env var a little less prominent anyway.

That said, CONTAINER_TOOL sounds reasonable to me!

@dagood dagood deleted the dev/dagood/docker-fallback branch March 4, 2024 17:03
@shawn-hurley
Copy link
Contributor

A follow up PR to rename and handle the existing PODMAN_BIN I think would be a good thing, if not can you file an issue and we can make the follow on ?

@dagood dagood mentioned this pull request Mar 4, 2024
Closed
@dagood
Copy link
Contributor Author

dagood commented Mar 4, 2024

Thanks, filed #168.

@eemcmullan eemcmullan mentioned this pull request Mar 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eemcmullan eemcmullan eemcmullan approved these changes

@pranavgaikwad pranavgaikwad pranavgaikwad approved these changes

@shawn-hurley shawn-hurley shawn-hurley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dagood @shawn-hurley @pranavgaikwad @eemcmullan